❮

[Commitfest 2022-07] Patch Triage: Waiting on Author

  • Jump to comment-1
    jchampion@timescale.com2022-07-26T19:26:59+00:00
    Hello all, I'm making my way through some stalled patches in Waiting on Author. If nothing changes by the end of this CF, I'd recommend marking these as Returned with Feedback. Patch authors CC'd. - jsonpath syntax extensions https://commitfest.postgresql.org/38/2482/ As a few people pointed out, this has not seen much review/interest in the roughly two years it's been posted, and it's needed a rebase since last CF. Daniel suggested that the featureset be split up for easier review during the 2021-11 triage. I recommend RwF with that suggestion. - Consider parallel for LATERAL subqueries having LIMIT/OFFSET https://commitfest.postgresql.org/38/2851/ Does this have a path forward? It's been Waiting on Author since the beginning of last CF, with open concerns from Tom about safety. - Parallel INSERT SELECT take 2 https://commitfest.postgresql.org/38/3143/ There was a lot of discussion early on in this patchset's life, and then it got caught in a rebase loop without review in August 2021. The thread has mostly gone dark since then and the patch does not apply. - Add callback table access method to reset filenode when dropping relation https://commitfest.postgresql.org/38/3073/ Heikki had some feedback back in Februrary but it hasn't been answered yet. It needs a rebase too. - Avoid orphaned dependencies https://commitfest.postgresql.org/38/3106/ Tom notes that this cannot be committed as-is; the thread has been silent since last CF. Last Author comment in January and needs a rebase. - Allow multiple recursive self-references https://commitfest.postgresql.org/38/3046/ There appears to be agreement that this is useful, but it looks like the patch needs some changes before it's committable. Last post from the Author was in January. - Push down time-related SQLValue functions to foreign server https://commitfest.postgresql.org/38/3289/ There's interest and engagement, but it's not committable as-is and needs a rebase. Last Author post in January. - Parallelize correlated subqueries that execute within each worker https://commitfest.postgresql.org/38/3246/ Patch needs to be fixed for FreeBSD; last Author post in January. - libpq compression https://commitfest.postgresql.org/38/3499/ Needs a rebase and response to feedback; mostly quiet since January. Thanks, --Jacob
    • Jump to comment-1
      x4mmm@yandex-team.ru2022-07-28T11:46:01+00:00
      > 27 июля 2022 г., в 00:26, Jacob Champion <jchampion@timescale.com> написал(а): > > - libpq compression > https://commitfest.postgresql.org/38/3499/ > > Needs a rebase and response to feedback; mostly quiet since January. Daniil is working on this, but currently he's on vacation. I think we should not mark patch as RwF and move it to next CF instead. Thank you! Best regards, Andrey Borodin.
      • Jump to comment-1
        jchampion@timescale.com2022-07-28T15:58:31+00:00
        On Thu, Jul 28, 2022 at 4:46 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote: > Daniil is working on this, but currently he's on vacation. > I think we should not mark patch as RwF and move it to next CF instead. Is there a downside to marking it RwF, from your perspective? As Robert pointed out upthread, it can be switched back at any time once Daniil's ready. Life happens; there isn't (or there shouldn't be) any shame in having a patch returned temporarily. But it is important that patches which aren't ready for review at the moment don't stick around for months. They take up reviewer time and need to be triaged continually. --Jacob
        • Jump to comment-1
          pryzby@telsasoft.com2022-08-01T15:51:05+00:00
          On Thu, Jul 28, 2022 at 08:58:31AM -0700, Jacob Champion wrote: > On Thu, Jul 28, 2022 at 4:46 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote: > > Daniil is working on this, but currently he's on vacation. > > I think we should not mark patch as RwF and move it to next CF instead. > > Is there a downside to marking it RwF, from your perspective? As > Robert pointed out upthread, it can be switched back at any time once > Daniil's ready. As someone interested in seeing the patch progress, I still think it may be better to close the patch record, which can be re-opened when it's ready to be reviewed. > They take up reviewer time and need to be triaged continually. Alternately: @Jacob: Is there any reason why it's necessary to do anything at all ? Does something bad happen if the patches are left in the current CF ? Why make not let patch authors (re) submit the patch for review when they're ready? Someone went to the effort to move it to the current CF, even though the patch wasn't ready to be reviewed. It'd be less work and avoid the process of "moving patches to the next CF" even though (at least in this case) it maybe shouldn't have even been in the current CF. Also, is there a place which lists all of an author's patches (current and historic)? I think people would be less adverse to having their patches closed if 1) they knew they could re-open them; and, 2) there were a list of patches and their disposition (not a separate list per commitfest, and not showing each patch duplicated for each CF that a patch was opened in). -- Justin
          • Jump to comment-1
            jchampion@timescale.com2022-08-01T16:30:39+00:00
            On 8/1/22 08:51, Justin Pryzby wrote: > @Jacob: Is there any reason why it's necessary to do anything at all ? > Does something bad happen if the patches are left in the current CF ? > Why make not let patch authors (re) submit the patch for review when they're > ready? Someone went to the effort to move it to the current CF, even though the > patch wasn't ready to be reviewed. It'd be less work and avoid the process of > "moving patches to the next CF" even though (at least in this case) it maybe > shouldn't have even been in the current CF. Maybe this is something to look into once we've implemented some more of the low-hanging usability features that people have asked for. But if we started doing it now, I'd expect the CFM's job to simply change from moving patches ahead to pinging people who have patches left behind, asking them if they meant to move the patches forward. I'm not convinced it'd be all that useful. > Also, is there a place which lists all of an author's patches (current and > historic)? I think people would be less adverse to having their patches closed > if 1) they knew they could re-open them; and, 2) there were a list of patches > and their disposition (not a separate list per commitfest, and not showing each > patch duplicated for each CF that a patch was opened in). This would be great to have. I have a patch in progress that introduces a "deferred" group, to make it more obvious the difference between a patch that has been Rejected and a patch that's simply Returned or Moved. Your suggestion would dovetail nicely with that, to able to see "all my deferred patches". --Jacob
            • Jump to comment-1
              robertmhaas@gmail.com2022-08-01T16:33:52+00:00
              On Mon, Aug 1, 2022 at 12:30 PM Jacob Champion <jchampion@timescale.com> wrote: > Maybe this is something to look into once we've implemented some more of > the low-hanging usability features that people have asked for. But if we > started doing it now, I'd expect the CFM's job to simply change from > moving patches ahead to pinging people who have patches left behind, > asking them if they meant to move the patches forward. I'm not convinced > it'd be all that useful. We really need to move to a system where it's the patch author's job to take some action if the patch is alive, rather than having the CM (or any other human being) pinging to find out whether it's dead. Having the default action for a patch be to carry it along to the next CF whether or not there are any signs of life is unproductive. -- Robert Haas EDB: http://www.enterprisedb.com
              • Jump to comment-1
                jchampion@timescale.com2022-08-01T16:43:06+00:00
                On 8/1/22 09:33, Robert Haas wrote: > We really need to move to a system where it's the patch author's job > to take some action if the patch is alive, rather than having the CM > (or any other human being) pinging to find out whether it's dead.> Having the default action for a patch be to carry it along to the next > CF whether or not there are any signs of life is unproductive. In the medium to long term, I agree with you. In the short term I want to see the features that help authors keep their patches alive (cfbot integration! automatic rebase reminders! automated rebase?) so that we're not just artificially raising the barrier to entry. People with plenty of time on their hands will be able to go through the motions of moving their patches ahead regardless of whether or not the patch is dead. --Jacob
                • Jump to comment-1
                  tgl@sss.pgh.pa.us2022-08-01T16:56:15+00:00
                  Jacob Champion <jchampion@timescale.com> writes: > On 8/1/22 09:33, Robert Haas wrote: >> We really need to move to a system where it's the patch author's job >> to take some action if the patch is alive, rather than having the CM >> (or any other human being) pinging to find out whether it's dead.> Having the default action for a patch be to carry it along to the next >> CF whether or not there are any signs of life is unproductive. > In the medium to long term, I agree with you. > In the short term I want to see the features that help authors keep > their patches alive (cfbot integration! automatic rebase reminders! > automated rebase?) so that we're not just artificially raising the > barrier to entry. People with plenty of time on their hands will be able > to go through the motions of moving their patches ahead regardless of > whether or not the patch is dead. Yeah, I don't want to introduce make-work into the process; there's more than enough real work involved. At minimum, a patch that's shown signs of life since the previous CF should be auto-advanced to the next one. regards, tom lane
                  • Jump to comment-1
                    robertmhaas@gmail.com2022-08-01T17:20:48+00:00
                    On Mon, Aug 1, 2022 at 12:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah, I don't want to introduce make-work into the process; there's > more than enough real work involved. At minimum, a patch that's > shown signs of life since the previous CF should be auto-advanced > to the next one. Maybe so, but we routinely have situations where a patch hasn't been updated in 3-6 months and we tentatively ask the author if it would be OK to mark it RwF, and they often say something like "please keep it alive for one more CF to see if I have time to work on it." IMHO, that creates the pretty ridiculous situation where CFMs are putting time into patches that the author isn't working on and hasn't worked on in a long time. The CF list isn't supposed to be a catalog of every patch somebody's thought about working on at any point in the last few years; it's supposed to be a list of things that need to be reviewed for possible commit. That's why it's called a COMMIT-fest. Back in the day, I booted patches out of the CF if they weren't updated within 4 days of a review being posted. I guess people found that too harsh, but now it feels like we've gone awfully far towards the other extreme. -- Robert Haas EDB: http://www.enterprisedb.com
                    • Jump to comment-1
                      tgl@sss.pgh.pa.us2022-08-01T17:33:27+00:00
                      Robert Haas <robertmhaas@gmail.com> writes: > Maybe so, but we routinely have situations where a patch hasn't been > updated in 3-6 months and we tentatively ask the author if it would be > OK to mark it RwF, and they often say something like "please keep it > alive for one more CF to see if I have time to work on it." Agreed, we don't want that. IMO the CF list isn't primarily a to-do list for patch authors; it's primarily a to-do list for reviewers and committers. The case that I'm concerned about here is where an author submits a patch and, through no fault of his/hers, it goes unreviewed for multiple CFs. As long as the author is keeping the patch refreshed per CI testing, I don't think additional work to express interest should be required from the author. Now admittedly, at some point we should decide that lack of review indicates that nobody else cares about this patch, in which case it should get booted with a "sorry, we're just not interested" resolution. But that can't happen quickly, because we're just drastically short of review manpower at all times. On the other hand, I'm quite willing to convert WOA state into RWF state quickly. The author can always resubmit, or resurrect the old CF entry, once they have something new for people to look at. regards, tom lane
                      • Jump to comment-1
                        robertmhaas@gmail.com2022-08-01T20:01:20+00:00
                        On Mon, Aug 1, 2022 at 1:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > On the other hand, I'm quite willing to convert WOA state into RWF > state quickly. The author can always resubmit, or resurrect the > old CF entry, once they have something new for people to look at. Right. This is what I'm on about. -- Robert Haas EDB: http://www.enterprisedb.com
    • Jump to comment-1
      houzj.fnst@fujitsu.com2022-07-28T02:09:43+00:00
      On Wednesday, July 27, 2022 3:27 AM Jacob Champion <jchampion@timescale.com> wrote: > > - Parallel INSERT SELECT take 2 > https://commitfest.postgresql.org/38/3143/ > > There was a lot of discussion early on in this patchset's life, and > then it got caught in a rebase loop without review in August 2021. The > thread has mostly gone dark since then and the patch does not apply. Sorry, I think we don't enough time to work on this recently. So please mark it as RWF and we will get back to this in the future. Best regards, Hou zj
      • Jump to comment-1
        jchampion@timescale.com2022-07-28T15:52:24+00:00
        On Wed, Jul 27, 2022 at 7:09 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > Sorry, I think we don't enough time to work on this recently. So please mark it as RWF and > we will get back to this in the future. Done, thanks! --Jacob
    • Jump to comment-1
      jtc331@gmail.com2022-07-26T23:47:31+00:00
      On Tue, Jul 26, 2022 at 3:27 PM Jacob Champion <jchampion@timescale.com> wrote: ... > - Consider parallel for LATERAL subqueries having LIMIT/OFFSET > https://commitfest.postgresql.org/38/2851/ > > Does this have a path forward? It's been Waiting on Author since the > beginning of last CF, with open concerns from Tom about safety. ... > - Parallelize correlated subqueries that execute within each worker > https://commitfest.postgresql.org/38/3246/ > > Patch needs to be fixed for FreeBSD; last Author post in January. These are both mine, and I'd hoped to work on them this CF, but I've been sufficiently busy that that hasn't happened. I'd like to just move these to the next CF. Thanks, James Coleman
      • Jump to comment-1
        robertmhaas@gmail.com2022-07-27T15:43:19+00:00
        On Tue, Jul 26, 2022 at 7:47 PM James Coleman <jtc331@gmail.com> wrote: > These are both mine, and I'd hoped to work on them this CF, but I've > been sufficiently busy that that hasn't happened. > > I'd like to just move these to the next CF. Well, if we mark them returned with feedback now, and you get time to work on them, you can always change the status back to something else at that point. That has the advantage that, if you don't get time to work on them, they're not cluttering up the next CF in the meantime. We're not doing a great job kicking things out of the CF when they are non-actionable, and thereby we are making life harder for ourselves collectively. -- Robert Haas EDB: http://www.enterprisedb.com
    • Jump to comment-1
      pryzby@telsasoft.com2022-07-26T23:20:19+00:00
      On Tue, Jul 26, 2022 at 12:26:59PM -0700, Jacob Champion wrote: > Hello all, > > I'm making my way through some stalled patches in Waiting on Author. If > nothing changes by the end of this CF, I'd recommend marking these > as Returned with Feedback. +1 I suggest that, if you send an email when marking as RWF, to mention that the existing patch record can be re-opened and moved to next CF. I'm aware that people may think that this isn't always a good idea, but it's nice to mention that it's possible. It's somewhat comparable to starting a new thread (preferably including a link to the earlier one). -- Justin
      • Jump to comment-1
        jchampion@timescale.com2022-07-26T23:30:01+00:00
        On 7/26/22 16:20, Justin Pryzby wrote: > I suggest that, if you send an email when marking as RWF, to mention that the > existing patch record can be re-opened and moved to next CF. > > I'm aware that people may think that this isn't always a good idea, but it's > nice to mention that it's possible. It's somewhat comparable to starting a new > thread (preferably including a link to the earlier one). Thanks, will do! --Jacob